Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue #5112 (line labels can render incorrectly on overzoomed tiles) #5120

Merged
merged 3 commits into from
Aug 23, 2017

Conversation

ChrisLoer
Copy link
Contributor

@ChrisLoer ChrisLoer commented Aug 8, 2017

Adds special handling for (rare) case that line geometry projects to a point behind the plane of the camera. Fixes issue #5112.

Before merging the PR, I want to build tests for the two cases here (multiple glyph and single-glyph labels). In development, I've been testing with the coordinates @asheemmamoowala provided in #5112, and modifying the style to make single-glyph labels.

/cc @asheemmamoowala @ansis @kkaefer

@ChrisLoer
Copy link
Contributor Author

Building tests was tricky using a GeoJSON source. For the single glyph special case I ended up just finding a vector tile that could reproduce the issue and I used the vector tile in the test case. Is that a reasonable approach?

Copy link
Contributor

@anandthakker anandthakker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ChrisLoer strategy looks good to me! Couple detail questions inline

const w = pos[3];
return {
point: new Point(pos[0] / w, pos[1] / w),
distanceToCamera: w
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested rename: signedDistanceFromCamera

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

const tileSegmentEnd = lineVertexArray.get(symbol.lineStartIndex + symbol.segment + 1);
const projectedVertex = project(tileSegmentEnd, posMatrix);
// We know the anchor will be in the viewport, but the end of the line segment may be
// past the plane of the camera, in which case we can use a point at any arbitrary (closer)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested edit: "past the plane of the camera" => "behind the plane of the camera"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -276,11 +290,19 @@ function placeGlyphsAlongLine(symbol,
return {};
}

function projectTruncatedLineSegment(previousTilePoint: Point, currentTilePoint: Point, previousProjectedPoint: Point, minimumLength: number, projectionMatrix: mat4) {
const projectedUnitVertex = project(previousTilePoint.add(previousTilePoint.sub(currentTilePoint)._unit()), projectionMatrix).point;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible for previousTilePoint + unitVector(previousTilePoint - currentTilePoint) to fall behind the camera, if previousTilePoint happened to be within one unit of the camera plane? If so, would that cause a similar problem to the one we're solving here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 That's a good question. I think the answer is "not under any reasonable conditions," but let me try to reason why:

  • Any point that's near the plane of the camera is very far "away" (in x/y terms) from the viewport
  • We're only placing glyphs whose anchor is within the viewport
  • For a label to reach from within the viewport to a line vertex at the edge of camera plane, it would need to be very long (much longer than could be shown on one screen)
  • A label with text-pitch-alignment: map would be the best candidate for being way longer than expected, because it would get stretched out more and more the closer it got to the plane of the camera. But that case wouldn't be affected by the "plane of the camera" logic, because it would be projected using a tile-based matrix (see logic in getLabelPlaneMatrix).
  • Therefore any label we're placing should fit on the line before the hypothetical vertex that's right at the edge of the camera plane, and we should never reach a point where we try to project past the hypothetical vertex.

I guess another way of thinking about it is: if we needed to draw a glyph that was on a line just next to the plane of the camera, we'd be very close to needing to draw a glyph that was just behind the plane of the camera, and we'd be hosed either way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 sounds legit to me -- thanks for spelling it out!

Adds special handling for (rare) case that line geometry projects to a point behind the plane of the camera.
Without the fix:
 - The "viewport-overzoomed" test would show "Figueroa St." jumbled in the lower half of the screen
 - The "viewport-overzoomed-single-glyph" test would show the letter "C" upside down
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants